Allow for custom labels on prometheus metrics#393
Allow for custom labels on prometheus metrics#393roblaszczak merged 4 commits intoThreeDotsLabs:masterfrom
Conversation
| } | ||
| } | ||
|
|
||
| func NewPrometheusMetricsBuilder(prometheusRegistry prometheus.Registerer, namespace string, subsystem string, opts ...Option) PrometheusMetricsBuilder { |
There was a problem hiding this comment.
Hey @matdurand, sorry about the delay. I wanted to comment last week and just realized I didn't do it. 🤦♂️
I have just one comment regarding the API — most Watermill components use a "Config" struct instead of the Option pattern, and I feel it would be a good idea to be consistent, just for the improved developer experience. This means we would need another constructor, something like NewPrometheusMetricsBuilderWithConfig. What do you think?
There was a problem hiding this comment.
Hey @m110, yeah sure, consistency is paramount. I replaced Option with config. I also renamed "custom" for "additional", just because it sounded better imo. Let me know if these changes are enough.
I also fixed an issue do deduplicate if you pass an additionalLabel that is already in the base labels. I had that issue locally in my test project because I needed to override the "publisher_name" label for a special case.
| return ctxLabels | ||
| } | ||
|
|
||
| type LabelComputeFn func(msgCtx context.Context) string |
There was a problem hiding this comment.
We might want to reflect on that signature. Is it enough to use only the context to get a label's value? Should we also pass the message pointer? For my use case, the additional labels are static, so I basically do this
metrics.MetricLabel{
Label: "service",
ComputeFn: func(ctx context.Context) string {
return "my_service"
},
},
but there might be a use case for getting something from the message itself as a label value.
…tom-labels # Conflicts: # components/metrics/builder.go
|
I took the liberty of renaming ComputeFn to ComputeValueFn and merging main into your branch. After tests will pass I'll merge it. Thanks for the contribution! |
3 participants
Solves #392